Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

kenshaw · 2024-11-12T00:28:22Z

Enable package to be usable with TinyGo.

Changes calls to reflect.Type.ConvertibleTo in decode.go to Go/TinyGo specific implementations. Basic examples work, but (some) unit tests fail as conversions to interface{} do not currently work in TinyGo.

Otherwise, all other functionality I've tested now appears to work properly: highlighted error output, cmd/ycat, the README.md examples, etc.

As an example, both Go and TinyGo are now able to use this package vis-a-vis this example taken from README.md:

package main

import (
	"fmt"
	"strings"

	"github.com/goccy/go-yaml"
)

func main() {
	yml := `
store:
  book:
    - author: john
      price: 10
    - author: ken
      price: 12
  bicycle:
    color: red
    price: 19.95
`
	path, err := yaml.PathString("$.store.book[*].author")
	if err != nil {
		//...
	}
	var authors []string
	if err := path.Read(strings.NewReader(yml), &authors); err != nil {
		//...
	}
	fmt.Println(authors)
}

Output:

ken@ken-desktop:~/g$ go run main.go
[john ken]
ken@ken-desktop:~/g$ tinygo run main.go
[john ken]
ken@ken-desktop:~/g$

(previously the tinygo run would panic)

Before submitting your PR, please confirm the following.

Describe the purpose for which you created this PR.
Create test code that corresponds to the modification

codecov-commenter · 2024-11-12T00:28:58Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.72%. Comparing base (ff5d41f) to head (e1eea91).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #522   +/-   ##
=======================================
  Coverage   79.71%   79.72%           
=======================================
  Files          18       19    +1     
  Lines        6374     6376    +2     
=======================================
+ Hits         5081     5083    +2     
  Misses        986      986           
  Partials      307      307

kenshaw · 2024-11-12T00:30:10Z

Hi, humbly requesting that this be added, as I have some command-line utilities that I'm switching over to to building with tinygo, and would like to use this package for YAML parsing.

Thanks in advance, and really appreciate the work on this package! Especially now that it has zero dependencies!

goccy · 2024-11-12T01:47:36Z

Thank you for your contribution.

Is there any public information regarding the lack of support for reflect.Type.ConvertibleTo in TinyGo ?
Additionally, if we include TinyGo as a build target, ensuring that it can be continuously built with TinyGo will be required, so we need to consider the maintenance cost.

kenshaw · 2024-11-12T02:02:59Z

Appreciate the quick response. I'm happy to contribute to the CI/CD build workflow if necessary, but I would not suggest making TinyGo as a major compatibility target and/or "supported".

For those using TinyGo, they are most likely already aware of TinyGo's limitations, and know they are working with subset of Go to begin with. As such, they likely would appreciate any functionality/support, and they will likely test their code code sufficiently enough.

The request for this to be merge is because it doesn't affect regular Go in any fashion, but at least enables using the package with TinyGo. It's worth noting that the API used in the actual conv_tinygo.go is Go's standard, public reflect API, it's actually possible to build/compile this path with regular Go, if one were so inclined (it just wouldn't work in all scenarios).

This PR is just a helpful workaround for those who are using TinyGo, as otherwise TinyGo's reflect package panics when calling the ConvertibleTo directly. Hopefully in some future release of TinyGo they will get ConvertibleTo (and the rest of the reflect package and standard library) working, and at that point this change would be revertible to just the standard API.

Enable package to be usable with [TinyGo](https://tinygo.org). Changes calls to reflect.Type.ConvertibleTo in decode.go to Go/TinyGo specific implementations. Basic examples work, but (some) unit tests fail as conversions to `interface{}` do not currently work in TinyGo. Otherwise, all other functionality I've tested now appears to work properly: highlighted error output, `cmd/ycat`, the README.md examples, etc. As an example, both Go and TinyGo are now able to use this package vis-a-vis this example taken from README.md: ```go package main import ( "fmt" "strings" "github.com/goccy/go-yaml" ) func main() { yml := ` store: book: - author: john price: 10 - author: ken price: 12 bicycle: color: red price: 19.95 ` path, err := yaml.PathString("$.store.book[*].author") if err != nil { //... } var authors []string if err := path.Read(strings.NewReader(yml), &authors); err != nil { //... } fmt.Println(authors) } ``` Output: ```sh ken@ken-desktop:~/g$ go run main.go [john ken] ken@ken-desktop:~/g$ tinygo run main.go [john ken] ken@ken-desktop:~/g$ ``` (previously the `tinygo run` would panic)

kenshaw · 2024-11-17T22:25:56Z

I've rebased the commit to the latest master. Would appreciate any feedback on what would be necessary to get this added to the source tree. Happy to do any work necessary to get this accepted. Thanks!

goccy · 2024-11-30T16:50:51Z

I agree with adding TinyGo support, but if we do so, we need to ensure that it can be continuously built and tested. Therefore, I believe we need to set up TinyGo builds and execute test code using the built artifacts. Once these are ready in the CI, we can merge this code. If preparing these is difficult, I can handle the setup myself. I have already built systems using TinyGo and WebAssembly 😃

kenshaw · 2024-11-30T22:39:34Z

Adding CI support for TinyGo is fairly trivial -- there's a GitHub action that will install TinyGo more or less the same as Go. The issue is that many of go-yaml's unit tests don't work with TinyGo, and would require rewriting, as the way the reflect package works in TinyGo is different than Go. As such, without heavy rework of the unit tests, many of the unit tests would not work with TinyGo.

I wouldn't expect this package to give some kind of "guarantee" to support TinyGo. The PR simply, at least, enables TinyGo to work with the package, whereas before it wouldn't even compile with TinyGo. It's the difference between being "completely 100% not working" and "semi-usable".

I don't think it would be fair to ask this project to support anything above/beyond that.

goccy · 2024-12-05T10:11:35Z

@kenshaw By the way, could you share the link to the issue you opened on TinyGo? Since this is a workaround, it should ideally be addressed on the TinyGo side. Therefore, it would be better to first check TinyGo's plans for resolving this issue.

kenshaw · 2024-12-12T21:01:01Z

I have not opened any PR on TinyGo, which I'm sure you looked for and were using as a rhetorical point. TinyGo doesn't claim to offer full Go support, and while I believe that project would eventually like to, it may be years (decades?) before TinyGo has full support for reflect, as it's one of the more challenging features of Go to implement with the model that TinyGo is taking. TinyGo is not a fully compatible with Go, and it may never be.

That said, I've always been extremely impressed with the go-yaml codebase, and can appreciate that many applications prefer to store configuration with YAML. The "other" well known YAML library is just fully incompatible with TinyGo, and I was hoping this PR could be adapted.

If it's not "in the cards" to adopt this PR, so to speak, then I understand.

kenshaw force-pushed the master branch from 616954b to e1eea91 Compare November 17, 2024 22:24

goccy added the waiting owner is waiting for a response label Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

kenshaw commented Nov 12, 2024

codecov-commenter commented Nov 12, 2024 •

edited

Loading

kenshaw commented Nov 12, 2024

goccy commented Nov 12, 2024

kenshaw commented Nov 12, 2024

kenshaw commented Nov 17, 2024

goccy commented Nov 30, 2024

kenshaw commented Nov 30, 2024

goccy commented Dec 5, 2024 •

edited

Loading

kenshaw commented Dec 12, 2024

Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

Are you sure you want to change the base?

Wrap reflect.Type.ConvertibleTo calls with Go/TinyGo implementations #522

Conversation

kenshaw commented Nov 12, 2024

codecov-commenter commented Nov 12, 2024 • edited Loading

Codecov Report

kenshaw commented Nov 12, 2024

goccy commented Nov 12, 2024

kenshaw commented Nov 12, 2024

kenshaw commented Nov 17, 2024

goccy commented Nov 30, 2024

kenshaw commented Nov 30, 2024

goccy commented Dec 5, 2024 • edited Loading

kenshaw commented Dec 12, 2024

codecov-commenter commented Nov 12, 2024 •

edited

Loading

goccy commented Dec 5, 2024 •

edited

Loading